Fix #121126: index out of bounds exceeds max value #123483
Conversation
When indexing an array with an index (u32) that exceeds the maximum value allowed by FieldIdx (default: 0xFFFF_FF00), although the compiler would detect the error, it would also cause a panic, which is a bug. I fixed it by adding a verification before calling the FieldIdx::from_u32(idx) method. This check ensures that if the idx value is greater than the maximum allowed value, it returns Option::None, similar to how other functions handle errors during the call to the project method of type Value.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TaKO8Ki (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
| let idx: u32 = prop.ecx.read_target_usize(idx).ok()?.try_into().ok()?; | ||
|
|
||
| let max: u32 = FieldIdx::MAX.index().try_into().ok()?; | ||
| if idx > max { |
There was a problem hiding this comment.
Please implement this as a method on all index types instead. Like try_from_u32 and then implement from_u32 by using that
There was a problem hiding this comment.
Can we have from_u32 returning Option instead of Self? I can't see a way to inform the caller that the operation was unsuccessful without using Option, Result or panicking. And I guess Result is not the way and panicking was what we had before with the assert()
There was a problem hiding this comment.
Hmm... we could just always return an option, good point. Yea lets do that instead of my idea of having both a try_from and a from method
There was a problem hiding this comment.
I have a doubt here. In this match inside project method from Value type, why do we always cast the usize idx to a u32 just to call then from_u32 when we have also a from_usize method?
There was a problem hiding this comment.
probably just an oversight. using from_usize is better
fmease
added
S-waiting-on-author
|
There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged. You can start a rebase with the following commands: The following commits are merge commits: |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #125821) made this pull request unmergeable. Please resolve the merge conflicts. |
|
A similar PR #125821 was already accepted, so I'm closing this one. |
When indexing an array with an index (u32) that exceeds the maximum value allowed by FieldIdx (default: 0xFFFF_FF00), although the compiler would detect the error, it would also cause a panic, which is a bug. I fixed it by adding a verification before calling the FieldIdx::from_u32(idx) method. This check ensures that if the idx value is greater than the maximum allowed value, it returns Option::None, similar to how other functions handle errors during the call to the project method of type Value.